link: bond: implement FromStr for bond enums#263
Conversation
Introduced support for parsing bond option enums from string: * `BondMode` * `BondArpValidate` * `BondArpAllTargets` * `BondPrimaryReselect` * `BondFailOverMac` * `BondXmitHashPolicy` * `BondAdSelect` All conversions use DecodeError as the error type. Unit tests are included for all seven FromStr implementations. Signed-off-by: Gris Ge <cnfourt@gmail.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #263 +/- ##
==========================================
+ Coverage 68.10% 68.17% +0.07%
==========================================
Files 144 146 +2
Lines 10103 10402 +299
==========================================
+ Hits 6881 7092 +211
- Misses 3222 3310 +88 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements std::str::FromStr for several bond-related enums, including BondMode, BondArpValidate, BondPrimaryReselect, BondXmitHashPolicy, BondArpAllTargets, BondFailOverMac, and BondAdSelect, along with corresponding unit tests. The reviewer noted that the from_str implementations use a non-idiomatic Rust pattern by wrapping the entire match block in Ok and using an early return for errors. It is recommended to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "balance-rr" => Self::BalanceRr, | ||
| "active-backup" => Self::ActiveBackup, | ||
| "balance-xor" => Self::BalanceXor, | ||
| "broadcast" => Self::Broadcast, | ||
| "802.3ad" => Self::Ieee8023Ad, | ||
| "balance-tlb" => Self::BalanceTlb, | ||
| "balance-alb" => Self::BalanceAlb, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond mode: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"balance-rr" => Ok(Self::BalanceRr),
"active-backup" => Ok(Self::ActiveBackup),
"balance-xor" => Ok(Self::BalanceXor),
"broadcast" => Ok(Self::Broadcast),
"802.3ad" => Ok(Self::Ieee8023Ad),
"balance-tlb" => Ok(Self::BalanceTlb),
"balance-alb" => Ok(Self::BalanceAlb),
_ => Err(DecodeError::from(format!("unknown bond mode: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "none" => Self::None, | ||
| "active" => Self::Active, | ||
| "backup" => Self::Backup, | ||
| "all" => Self::All, | ||
| "filter" => Self::Filter, | ||
| "filter_active" => Self::FilterActive, | ||
| "filter_backup" => Self::FilterBackup, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond arp validate: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"none" => Ok(Self::None),
"active" => Ok(Self::Active),
"backup" => Ok(Self::Backup),
"all" => Ok(Self::All),
"filter" => Ok(Self::Filter),
"filter_active" => Ok(Self::FilterActive),
"filter_backup" => Ok(Self::FilterBackup),
_ => Err(DecodeError::from(format!("unknown bond arp validate: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "always" => Self::Always, | ||
| "better" => Self::Better, | ||
| "failure" => Self::Failure, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond primary reselect: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"always" => Ok(Self::Always),
"better" => Ok(Self::Better),
"failure" => Ok(Self::Failure),
_ => Err(DecodeError::from(format!("unknown bond primary reselect: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "layer2" => Self::Layer2, | ||
| "layer34" => Self::Layer34, | ||
| "layer23" => Self::Layer23, | ||
| "encap23" => Self::Encap23, | ||
| "encap34" => Self::Encap34, | ||
| "vlan-src-mac" => Self::VlanSrcMac, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond xmit hash policy: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"layer2" => Ok(Self::Layer2),
"layer34" => Ok(Self::Layer34),
"layer23" => Ok(Self::Layer23),
"encap23" => Ok(Self::Encap23),
"encap34" => Ok(Self::Encap34),
"vlan-src-mac" => Ok(Self::VlanSrcMac),
_ => Err(DecodeError::from(format!("unknown bond xmit hash policy: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "any" => Self::Any, | ||
| "all" => Self::All, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond arp all targets: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"any" => Ok(Self::Any),
"all" => Ok(Self::All),
_ => Err(DecodeError::from(format!("unknown bond arp all targets: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "none" => Self::None, | ||
| "active" => Self::Active, | ||
| "follow" => Self::Follow, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond fail over mac: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"none" => Ok(Self::None),
"active" => Ok(Self::Active),
"follow" => Ok(Self::Follow),
_ => Err(DecodeError::from(format!("unknown bond fail over mac: {s}"))),
}
}| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Ok(match s { | ||
| "stable" => Self::Stable, | ||
| "bandwidth" => Self::Bandwidth, | ||
| "count" => Self::Count, | ||
| _ => { | ||
| return Err(DecodeError::from(format!( | ||
| "unknown bond ad select: {s}" | ||
| ))) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Using Ok(match s { ... _ => return Err(...) }) is non-idiomatic in Rust. It is cleaner and more idiomatic to match directly to Result<Self, Self::Err> by wrapping each successful arm in Ok and the fallback arm in Err.
fn from_str(s: &str) -> Result<Self, Self::Err> {
match s {
"stable" => Ok(Self::Stable),
"bandwidth" => Ok(Self::Bandwidth),
"count" => Ok(Self::Count),
_ => Err(DecodeError::from(format!("unknown bond ad select: {s}"))),
}
}1 participant
Introduced support for parsing bond option enums from string:
BondModeBondArpValidateBondArpAllTargetsBondPrimaryReselectBondFailOverMacBondXmitHashPolicyBondAdSelectAll conversions use DecodeError as the error type.
Unit tests are included for all seven FromStr implementations.